Packages: Add new @wordpress/data-controls package. #15435

nerrad · 2019-05-05T01:47:49Z

Description

This is a new package triggered in part by this and this comment.

The @wordpress/data-controls package exposes a set of common controls consumers can use when registering custom stores. In the initial iteration the following controls are included:

select: For selecting from another data store in the registry.
dispatch: For dispatching actions to another store in the registry.
apiFetch: For doing a request via the @wordpress/api-fetch module.

Notes:

The select control has been made resolver aware. Essentially this means if the named selector has a sibling resolver registered in the given store, then a promise is returned that resolves to the value for the selector after any necessary resolution is completed. If the selector does not have a resolver sibling then it is called directly. This allows for client code to not have to worry about resolution when using this control (Note: This requires a change in @wordpress/data: see @wordpress/data: Expose hasResolver property on returned selectors #15436).
I've implemented this new package with the @wordpress/editor. Primarily I did this so that things could be tested but I'm on the fence with whether this should be done in its own pull. So I'm looking from feedback from reviewers on this. It turns out the the changes aren't that much (other than eliminating code 🎉) so I think it'd be good to merge with an initial implementation of the package.

Todos:

Write package
Update main package.json for the mono-repo
Do pull for @wordpress/data exposing whether a selector has a resolver (see @wordpress/data: Expose hasResolver property on returned selectors #15436)
Write README.md
Write CHANGELOG.md
Update docs builder and make sure auto-generated docs look okay.
Add tests
Implement this somewhere (@wordpress/editor) to ensure api is usable and works as expected.
If the above ^^ stays in this branch and is released with this pull, then the appropriate CHANGELOG.md and tests will need updated.

How has this been tested?

New unit tests added
Implemented with @wordpress/editor and ensure the editor continues to work via user testing.
e2e tests should still pass with this work.

Types of changes

This is a non-breaking change introducing a new package.
As an example of implementation, this has been implemented with the core/editor store. Thus the editor is impacted but impacted areas are covered by e2e and unit tests.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

packages/data-controls/src/index.js

gziolo

I guess this should be reviewed by @youknowriad or @aduth. I can try myself but I didn't follow recent changes in this area.

I left my comments related to the package itself. Everything looks great, there are some tiny things to updated which I highlighted.

gziolo · 2019-05-20T08:01:21Z

bin/update-readmes.js

@@ -17,6 +17,7 @@ const packages = [
 		'Autogenerated selectors': 'src/selectors.js',
 	} ],
 	'data',
+	'data-controls',


@nosolosw, have you had a chance to revisit this list so we could use blacklist instead to make it easier to add new packages?

It has grown on me that an allow list works better in this case for two reasons:

I know we've talked about detecting the entry point from package.json. Note, however, how some packages will need more than this (example: core-data. This will require an allow list anyway.

Using a deny list will do unnecessary extra work. Currently, docgen will report an error if it doesn't find a token in a README file, but this can be improved to gracefully continue. However, we'll need parsing the package entry point files anyway to know whether there is a token or not - wasting those cycles.

Adding/Removing a package is not a common operation, so I think is fine to require authors to do this explicitely if they want API doc generation.

packages/data-controls/package.json

packages/data-controls/README.md

aduth · 2019-05-21T16:09:48Z

First thought: Is apiFetch too specific to WordPress use case for broad applicability in such a package? I could imagine fetch being useful, and I forget the exact reasons @wordpress/api-fetch is not marketed as a generic fetch utility.

aduth · 2019-05-21T16:13:59Z

Thought: Should these (at least select, dispatch) be offered by the data package itself? Possibly also by default? The more I think on it, the more I think "No" (prefer opt-in), but mentioning as it occurs to me. Similarly there's the thought of respective packages offering controls (api-fetch), but I also think this bloats the scope of the modules to become aware of the data package and its specific mechanics.

aduth · 2019-05-21T16:18:44Z

This allows for client code to not have to worry about resolution when using this control

I like this enhancement 👍

aduth

I'd be more okay to start with just select and dispatch, but I could be convinced of apiFetch if the arguments are made. The code looks good.

aduth

We need to update this, since it has a default export. Otherwise the window global doesn't work as expected (wp.dataControls.default):

gutenberg/webpack.config.js

Lines 71 to 78 in 8960509

    
           new LibraryExportDefaultPlugin( [ 
        
           	'api-fetch', 
        
           	'deprecated', 
        
           	'dom-ready', 
        
           	'redux-routine', 
        
           	'token-list', 
        
           	'shortcode', 
        
           ].map( camelCaseDash ) ),

Although, this is a unique case of needing both default and named exports. How do you propose this might be handled from the global? Is referencing the controls object by .default acceptable? It doesn't seem quite so obvious to me.

nerrad · 2019-05-22T00:16:53Z

I'd be more okay to start with just select and dispatch, but I could be convinced of apiFetch if the arguments are made.

Arguments for:

arbitrary requests to an api is one of the more common use-cases for a control. Having a default utility in place for this is helpful for the end user.
The package is only useful for consumers of the @wordpress/data package and registries implementing the controls plugin (available by default on the exposed defaultRegistry). Thus it makes sense that a fetch control would use the apiFetch utility from the @wordpress namespace.
There is immediate benefit within the GB project using this library as nearly always these three controls will be sufficient.
exposing these controls does not preclude implementors overriding or supplementing with their own controls. It just helps expose a set of sensible defaults that in most cases will be sufficient.

Although, this is a unique case of needing both default and named exports. How do you propose this might be handled from the global? Is referencing the controls object by .default acceptable? It doesn't seem quite so obvious to me.

Interesting, this was a specific about the build process I wasn't aware of. I'm also a bit surprised that the implementation here of the new package (in @wordpress/editor) didn't break because of that.

Typical usage will be importing the default for registering with the store (as done here) Are you thinking it'd be better to export it as a named export so that it's a bit more evident it's purpose on the global (i.e. wp.dataControls.controls)?

nerrad · 2019-05-22T00:32:27Z

Oh I see, the LibraryExportDefaultPlugin exports defaults onto the named global (eg. wp.apiFetch as opposed to wp.apiFetch.default)? I don't use ES5 that much so I don't notice these things. So to clarify, this means:

With configuration in webpack plugin

wp.dataControls for default
~~wp.dataControls.select, wp.dataControls.dispatch... etc for named exports?~~ Edit: I tried this, no go.

Without configuration in webpack plugin

wp.dataControls.default for default
wp.dataControls.select, wp.dataControls.dispatch ...etc for named exports?

- to use new data-controls package

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

Follows repo conventions about using Master as the version heading for unreleased changes.

This reverts commit 36a0b98.

aduth · 2019-05-22T12:41:06Z

I agree controls as a named export seems sensible (arguably even preferable, since I don't know that it's as intuitive as the default export).

jorgefilipecosta · 2019-05-22T13:10:27Z

Merging this PR as it seems ready and was approved, and I have a PR depending on it that I would like to merge. If there are is any other improvement I think it can be shipped in a follow-up PR.

jorgefilipecosta · 2019-05-22T13:12:35Z

@jorgefilipecosta since you've implemented this in another branch, do you have a preference or some ideas here?

Sorry, I missed this question, personally like the version that was proposed:
wp.dataControls.controls for the controls object.
wp.dataControls.select
wp.dataControls.dispatch
wp.dataControls.apiFetch

nerrad self-assigned this May 5, 2019

nerrad added [Package] Data Controls /packages/data-controls [Type] Enhancement A suggestion for improvement. npm Packages Related to npm packages and removed npm Packages Related to npm packages labels May 5, 2019

nerrad mentioned this pull request May 5, 2019

@wordpress/data: Expose hasResolver property on returned selectors #15436

Merged

7 tasks

nerrad commented May 5, 2019

View reviewed changes

packages/data-controls/src/index.js Outdated Show resolved Hide resolved

nerrad force-pushed the FET/controls-package branch from 9a9a4a1 to b47c02c Compare May 19, 2019 14:56

nerrad marked this pull request as ready for review May 19, 2019 16:39

nerrad requested review from aduth, ajitbohra, chrisvanpatten, gziolo, mkaz, noisysocks, oandregal, notnownikki, ntwb, talldan and youknowriad as code owners May 19, 2019 16:39

gziolo reviewed May 20, 2019

View reviewed changes

aduth approved these changes May 21, 2019

View reviewed changes

aduth requested changes May 21, 2019

View reviewed changes

jorgefilipecosta force-pushed the FET/controls-package branch from f7b2006 to 8ce6b53 Compare May 21, 2019 17:24

jorgefilipecosta mentioned this pull request May 21, 2019

Connnect gutenberg widgets screen to widget area endpoints #15074

Merged

nerrad and others added 20 commits May 22, 2019 08:38

updated manifest from doc generation

63b00c5

updated package-lock.json

57ea88d

not sure why docs build tool is doing this.

9e76c48

use ternary

1642349

data-controls added to manifest by docs build tool

dff65a5

update README for editor package

0349090

updates to editor store actions tests

48107e0

- to use new data-controls package

add missing @example tag and regenerate docs

5748e1c

add tests for package

e9dc0a6

fix typo in changelog for package name

1e0070f

fix typo for package name in readme

a6467e5

more typos

d62acd3

Use lower version for initial release.

c7b2c61

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

Add heading for API.

8689a77

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

Update CHANGELOG.md version reference.

d187fcc

Follows repo conventions about using Master as the version heading for unreleased changes.

add data-controls to webpack plugin config

b9d38ed

Revert "add data-controls to webpack plugin config"

9ea3413

This reverts commit 36a0b98.

use named export for controls object

ca4a16c

update docs

03823a0

woop fix import on tests

5dfee2c

nerrad force-pushed the FET/controls-package branch from 5afd069 to 5dfee2c Compare May 22, 2019 12:39

aduth approved these changes May 22, 2019

View reviewed changes

jorgefilipecosta merged commit 084e92b into master May 22, 2019

youknowriad added this to the 5.8 (Gutenberg) milestone May 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packages: Add new @wordpress/data-controls package. #15435

Packages: Add new @wordpress/data-controls package. #15435

nerrad commented May 5, 2019 •

edited

Loading

gziolo left a comment

gziolo May 20, 2019

oandregal May 20, 2019

aduth commented May 21, 2019

aduth commented May 21, 2019

aduth commented May 21, 2019

aduth left a comment

aduth left a comment •

edited

Loading

nerrad commented May 22, 2019 •

edited

Loading

nerrad commented May 22, 2019 •

edited

Loading

aduth commented May 22, 2019

jorgefilipecosta commented May 22, 2019

jorgefilipecosta commented May 22, 2019

	new LibraryExportDefaultPlugin( [
	'api-fetch',
	'deprecated',
	'dom-ready',
	'redux-routine',
	'token-list',
	'shortcode',
	].map( camelCaseDash ) ),

Packages: Add new @wordpress/data-controls package. #15435

Packages: Add new @wordpress/data-controls package. #15435

Conversation

nerrad commented May 5, 2019 • edited Loading

Description

Notes:

Todos:

How has this been tested?

Types of changes

Checklist:

gziolo left a comment

Choose a reason for hiding this comment

gziolo May 20, 2019

Choose a reason for hiding this comment

oandregal May 20, 2019

Choose a reason for hiding this comment

aduth commented May 21, 2019

aduth commented May 21, 2019

aduth commented May 21, 2019

aduth left a comment

Choose a reason for hiding this comment

aduth left a comment • edited Loading

Choose a reason for hiding this comment

nerrad commented May 22, 2019 • edited Loading

nerrad commented May 22, 2019 • edited Loading

aduth commented May 22, 2019

jorgefilipecosta commented May 22, 2019

jorgefilipecosta commented May 22, 2019

nerrad commented May 5, 2019 •

edited

Loading

aduth left a comment •

edited

Loading

nerrad commented May 22, 2019 •

edited

Loading

nerrad commented May 22, 2019 •

edited

Loading